Support Node-like module loaders #1103

isaacs · 2012-12-26T17:31:04Z

Set module.exports to the jQuery object in Node, and other module loaders like it.

Do not unnecessarily pollute the global namespace in AMD/Node/CommonJS module loaders.

rwaldron · 2012-12-26T18:22:36Z

http://bugs.jquery.com/ticket/13119

rwaldron · 2012-12-26T18:26:18Z

src/exports.js

+
+// Otherwise expose jQuery to the global object as usual
+else {
+  window.jQuery = window.$ = jQuery;


Indents need to be tabs, not spaces

rwaldron · 2012-12-26T18:29:51Z

The else statements should start on the same line as the preceeding IfStatement's closing curly, with the comments immediately inside the block. Thanks :)

isaacs · 2012-12-26T20:39:24Z

Style fixups squashed into 90cc01f5bf3d360783f88daf56000e94b0f6eae2.

Krinkle · 2012-12-31T19:06:46Z

+1

dmethvin · 2013-01-20T23:41:49Z

Where does this pull request stand? @rwldrn did you have other concerns?

dcherman · 2013-01-21T00:12:30Z

Is there a good reason to not expose jQuery/$ as a global when an AMD loader is present like the current PR does? That's a breaking change for AMD users FWIW.

For comparison, other AMD compatible utilities like Lodash, Hogan.js, and the Backbone/Underscore AMD versions maintained by @jrburke still create a global even in the presence of an AMD loader.

jrburke · 2013-01-21T00:38:37Z

Right, the global should still be exposed at least in the AMD case. Probably for the commonjs style case if they expect to run in the browser. As we have seen with other base libraries that support "plugins", like backbone and underscore, those plugins normally assume a global for the base library and attach themselves to it. They are slower to adopt a module registration pattern.

domenic · 2013-01-28T09:22:23Z

Ping @isaacs; up for incorporating the above concerns? I can do it instead.

isaacs · 2013-01-28T16:39:54Z

So, the consensus is that in the AMD case, it should still expose a global? I have no opinions there, so I'm happy to change it.

In the node-like case, it should definitely not create a global. That's considered bad behavior.

domenic · 2013-01-28T17:02:02Z

But in the Node-like case, it's going to be used in the browser, and if we want to be able to use it along with jQuery plugins, it needs to create a global :-/.

isaacs · 2013-01-28T17:12:15Z

Really? In the browser, there's going to be a module object with an exports member?

What node-like module loaders are there in the browser that depend on globals being created? Browserify certainly doesn't.

jrburke · 2013-01-28T17:42:47Z

It would be good to understand the actual use case for needing this. This does not seem to be Node-like, but more CommonJS-like browser loaders. I thought the trend in the folks that liked those loaders was to always do builds. And in that case, it seems fine to just suggest those loaders use AMD as their transport format.

isaacs · 2013-01-28T17:57:43Z

@jrburke People are using jquery in Node today. But it's a huge confusing pita, because npm install jquery is not jQuery, it's a jsdom thing, and out of date. (jQuery has some things that are not browser-specific, I guess? I don't know, it seems silly to me, but whatever, people do silly things sometimes and it's fine :)

If someone builds jQuery using a tool that presents a Node module interface, then that tool must not be depending on leaking globals, since this is simply not done in the Node module world. Whether it uses an AMD or AMD-like transport mechanism, the API provided to the user clearly does not depend on leaking globals. What the user will expect is that var $ = require('jquery') loads the jQuery object into their local $ var, and does not create any globals whatsoever.

This does not seem to be Node-like, but more CommonJS-like browser loaders.

CommonJS is dead. Let's be honest here, there only "CommonJS" module system in use in any relevant way is Node, which never implemented more than the barest minimum of the CommonJS spec, and then went wildly off the rails as that organization designed-by-committee itself into obsolescence years ago. And the only build tool that we're actually talking about is Browserify, which presents a Node module interface for use on the browser, by generating AMD-like boilerplate that it wraps around the code.

domenic · 2013-01-28T18:09:02Z

The use case here is npm + browserify. Simple as that.

With that in mind, leaking globals is desired, because you might use npm + browserify+ jQuery + a jQuery plugin, and jQuery plugins depend on the jQuery global.

There is a difference in "what is done" in the browserify world and in the pure Node.js world. In the browserify world, globals are still a fact of life, and must be accomodated---especially for packages like jQuery that have a plugin architecture dependent entirely on the global namespace.

Put another way, what the user expects is that not only does var $ = require("jquery") load the jquery function, but also that <script src="jquery.color.js"></script> works on the same page.

jrburke · 2013-01-28T18:11:46Z

@isaacs: yeah, the "npm in jquery" points to not putting front end code in npm. While not having globals would be great for a module system in the browser, it turns out is a huge support headache since module support in client code is so uneven.

So this change seems unnecessary. Browserify would do better by just updating its wrapping to AMD. We don't need to have more format wars around modules. Node's module system is not a good fit for direct use in the browser, and there will always need to be a bundled format for use in the browser. For that, AMD has the mindshare. Let's move on to solving higher level problems, or helping ES to get it right.

domenic · 2013-01-28T18:15:33Z

I strongly disagree, @jrburke. We have used npm packages and node modules with great success in the browser at multiple companies and in several projects. Module support in client code is quite good, and we have with great success used npm to manage our client side dependencies and installed dozens of modules from npm (hundreds if you count transitive dependencies). You may not enjoy that way of working, possibly because it does not use a format you created, but many others do, and trying to squash what is unequivocally a good change seems mean-spirited and counterproductive.

isaacs · 2013-01-28T18:20:39Z

Global re-exposed on a3601e361418e3a7e91d6ad03be9b87a4870ad45.

Any other issues to address?

"npm in jquery" points to not putting front end code in npm.
...
Node's module system is not a good fit for direct use in the browser, and there will always need to be a bundled format for use in the browser.

Ok, that's nice. Take it up with the people using Browserify. Unless your bottom line is that jQuery should not support the Node module environment at all, and this pull request should be rejected, it is out of scope for this discussion.

Whatever the case, people ARE using jquery in Node now on the server, and they ARE using jquery in Browserify, and leaking a global is bad behavior in a Node module environment. This pull request is only about correcting that bad behavior, and getting us closer to the jQuery team owning jQuery in npm.

dcherman · 2013-01-28T18:20:49Z

src/exports.js

@@ -15,4 +12,14 @@ window.jQuery = window.$ = jQuery;
 // noConflict to hide this version of jQuery, it will work.
 if ( typeof define === "function" && define.amd && define.amd.jQuery ) {
 	define( "jquery", [], function () { return jQuery; } );
+	window.jQuery = window.$ = jQuery;
+} else if ( typeof module === "object" && typeof module.exports === "object" ) {


If you check this condition first, then in the else you should be guaranteed to have to export the globals so you wouldn't need to duplicate that code.

jrburke · 2013-01-28T18:53:07Z

@domenic: sorry, I mixed some things in my message. If you want to use npm to move code around, that is fine. That issue is separate from the main one here, I apologize for getting distracted.

For whatever you use to deploy the code to the browser, the bundling format should probably just use AMD though.

@isaacs: yes, my comments were suggesting that if this pull request to support the node environment was so that it would work in a node environment, that is out of scope for jQuery because jQuery cannot be used in node as-is.

document and window are not in node. If they do show up there, it is because of a node module providing globals for them, and if globals are in play, this is likely just a browser simulation environment that does not need jQuery as a node module. In addition, expecting all browser code, like jQuery plugins, to fit into the "no globals" pattern has not worked. We tried that in AMD, and got real data that it causes more support headaches for the library (what happened with underscore/backbone).

So, if this pull request is for:

running jQuery in node: that is likely the browser simulation environment, and jQuery does not need this change. There are much larger issues with running jQuery in node, and other code run will likely want jQuery as a global. Maybe there are use cases I don't know about though that do not fit into this description.
supporting node module systems in the browser: those systems already need a bundling format. AMD has the mindshare there, and jQuery already supports it. Trying to support lots of different module formats in the browser seems counterproductive at this point, unless it is for ES modules.

isaacs · 2013-01-28T19:45:19Z

running jQuery in node: that is likely the browser simulation environment, and jQuery does not need this change. There are much larger issues with running jQuery in node, and other code run will likely want jQuery as a global. Maybe there are use cases I don't know about though that do not fit into this description.

I have received complaints that the jquery module on npm includes a browser simulation, and that it leaks globals, on github, twitter, and via email. My answer to these requests was traditionally, "Not my problem, take it up with jQuery/etc." but eventually I decided to get involved with this pull req and discussions with the jquery team and the maintainers of the existing npm modules named "jquery".

You are saying that they're going to need to use a global jQuery anyway, but they're saying otherwise, and I'm inclined to believe them rather than you regarding what they need.

supporting node module systems in the browser: those systems already need a bundling format. AMD has the mindshare there, and jQuery already supports it. Trying to support lots of different module formats in the browser seems counterproductive at this point, unless it is for ES modules.

I don't really know what "mindshare" actually means in this context, and I'm confused as to why AMD's possession of this "mindshare" would mean that real world node and Browserify users who are complaining about this should be forced to leak globals when their module system does not need this behavior.

Just to be clear, it sounds like your position on this entire pull request is that you are against it, in principle, because it allows people to do things that you think are a bad idea (to wit: a. putting jquery on npm, b. using jquery in node, and c. bundling jquery with a loader other than AMD).

However, some other people are very much for this change (in principle, that is; nits regarding the implementation notwithstanding), and I fail to see how your concerns are relevant to them, or how this minimal patch will affect you, or any users of your libraries. If you can show how it actually negatively impacts AMD, or AMD users, or any use case you actually have, then great, let's hear it. But if your only complaint is that it'll help people who are doing a thing you disagree with, then that's really not any of your business, afaict.

dmethvin · 2013-01-28T20:52:49Z

src/sizzle

@@ -1 +1 @@
-Subproject commit c0d9badeb5dac649d0ba7824efaebbe4155369b5
+Subproject commit 19c7b3440385c9f628a7bc1c5769f6946fcc6887


This shouldn't be here, there were no Sizzle changes.

I was able to pull this off without affecting the rest of the patch. I can land it if you're good to go

*by "able", I just used Tower's "discard local changes" ;)

jrburke · 2013-01-28T20:53:44Z

@isaacs: great, thanks for providing more info. Maybe you and the jQuery team already discussed those use cases, so I'm sorry if this is a rehash. Nice to have them written down future reference though.

Hiding the global is likely to generate support requests to jQuery when the user in the browser global context you describe then tries to load a jquery plugin that wants jquery as a global. I'm not suggesting anything about AMD in this case, only that it is a module format that has tried loading browser code and we found data to that effect. It is a real data point from something that tried something similar. @domenic shared this particular concern too, and he seems to be a user that would benefit from this change.

While I can appreciate you get a lot of pings from node programmers complaining about what may seem a simple problem, it does not mean those complaints correctly identify the underlying problem. It may really be about a larger issue with simulating browsers in node, the expectation to being able to use just any browser code in node and/or the difficulties of not having a universal module system in JS. I still feel the use case is a bit blurry, but you may have already worked that out with the jQuery folks.

Just to be clear, it sounds like your position on this entire pull request is that you are against it, in principle, because it allows people to do things that you think are a bad idea (to wit: a. putting jquery on npm, b. using jquery in node, and c. bundling jquery with a loader other than AMD).

a. no, I already said the npm issue is not part of this issue and apologized for distraction. I should not have focused on that part of your previous comment.

b. if it creates support requests for jQuery because the use case and implementation is flawed, then yes. At the very least removing the global does seem flawed based on previous data.

c. similarly, this is a support issue. Lots of people have invented lots of ways to bundle client code into modules. jQuery cannot expect to support them all. Perhaps the jQuery people feel this change meets their minimum bar, which is fine. However it also seems like a reasonable answer to just suggest claim AMD for client bundling since it has already been tested and known to work, and does not add more to their support cost.

So, while you may have read my feedback as some "only do things my way" screed, my concerns were more about not having enough context about what this will really solve, you understanding of the second level effects, and the support cost it places on jQuery, particularly if it goes in without the global export.

Hopefully that summarizes my concerns, I leave it to the jQuery folks to sort out, unless there are questions directed to me.

rwaldron · 2013-01-28T20:53:46Z

@dmethvin I pulled this and seems to have all its ducks in a row—I had to discard the changes to Sizzle submodule commit pointer, but that's no big deal. Leaving this for you to check before landing.

dmethvin · 2013-01-28T20:54:15Z

src/outro.js

@@ -1,2 +1,2 @@

-})( window );
+})( this );


If "use strict" is in effect, this isn't set to the global object is it? Also, window and the global object aren't always identical, I forget the specifics but IE 6/7/8 may have issues.

That would only be a concern if jQuery source was naively concattenated with other files that included bare "use strict"; not sure how much of a concern that is. If so, then I believe (false || eval)("this") will do the trick.

If someone were using this under node, would they want what jQuery considers to be the window object to be the global object, or would it be better for them to create a global window object and pass that in with whatever methods were needed? It seems like the former requires more pollution of the global namespace.

That's a good point. In my experience using jQuery in Node directly (as opposed to in browserify), I always load it in a sandbox which has the global equal to a jsdom window. So it doesn't make any difference in that case. And it doesn't make any difference in the browserify case either, where they're identical. /shrug

In Node, this is the exports object (for reasons of historical weirdness). But if we're not setting a global reference, then it doesn't matter.

The surest "global getter" that I've ever seen is (function () { return this; }).call(null). You could do something like (function () { return this; }).call(null).window so that it's empty if there's no global window defined, or the global window in any web browser.

Can we rely on this being in the global scope? What if a site concatenates this and other modules in one request and wraps it in a loader closure?

Definite possibility

@Krinkle Would you prefer doing something like typeof window === 'undefined' ? this : window?

@isaacs Hm.. maybe, but why can't it be just window. We only use it in a browser environment, right? In Node for example we set exports, or are we (ab)using the historical this being the exports object in Node?

@Krinkle Well, no, that's the point. When run in Node, jquery requires a window to already be defined, or it throws an undefined reference at this point. It looks like you really do want it to be the global though, not some random object. (There's window.String stuff, it looks like.)

domenic · 2013-02-20T21:01:05Z

What's left to get this landed? If you want a foolproof global, just do the usual ugly Function("return this")(). Anything else?

rwaldron · 2013-02-20T21:04:10Z

Aside from any concerns @dmethvin may have, it needs to be rebased.

dmethvin · 2013-02-20T22:32:04Z

No remaining concerns here, and it would be great to land this before the next beta. Last call..

rwaldron · 2013-02-20T22:36:55Z

Great! @isaacs can you rebase? Thanks!

Also, do not pollute the global namespace in Node module loaders.

isaacs · 2013-02-20T22:39:49Z

Rebased onto upstream master.

rwaldron reviewed Dec 26, 2012
View reviewed changes

gibson042 mentioned this pull request Jan 3, 2013

Added commonjs type exports jquery/sizzle#183

Closed

dcherman reviewed Jan 28, 2013
View reviewed changes

dmethvin reviewed Jan 28, 2013
View reviewed changes

Support node-like module loaders

b7570de

Also, do not pollute the global namespace in Node module loaders.

rwaldron closed this in a128355 Feb 21, 2013

coolaj86 mentioned this pull request Jun 8, 2013

The state and future of the jquery npm package(s) HorseAJ86/node-jquery#59

Closed

kevinsawicki mentioned this pull request Sep 20, 2013

Update jquery atom/atom#865

Closed

lock bot locked as resolved and limited conversation to collaborators Jan 22, 2019

		@@ -1 +1 @@
		Subproject commit c0d9badeb5dac649d0ba7824efaebbe4155369b5
		Subproject commit 19c7b3440385c9f628a7bc1c5769f6946fcc6887

Support Node-like module loaders #1103

Support Node-like module loaders #1103

Conversation

isaacs commented Dec 26, 2012

rwaldron commented Dec 26, 2012

Choose a reason for hiding this comment

rwaldron commented Dec 26, 2012

isaacs commented Dec 26, 2012

Krinkle commented Dec 31, 2012

dmethvin commented Jan 20, 2013

dcherman commented Jan 21, 2013

jrburke commented Jan 21, 2013

domenic commented Jan 28, 2013

isaacs commented Jan 28, 2013

domenic commented Jan 28, 2013

isaacs commented Jan 28, 2013

jrburke commented Jan 28, 2013

isaacs commented Jan 28, 2013

domenic commented Jan 28, 2013

jrburke commented Jan 28, 2013

domenic commented Jan 28, 2013

isaacs commented Jan 28, 2013

Choose a reason for hiding this comment

jrburke commented Jan 28, 2013

isaacs commented Jan 28, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrburke commented Jan 28, 2013

rwaldron commented Jan 28, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domenic commented Feb 20, 2013

rwaldron commented Feb 20, 2013

dmethvin commented Feb 20, 2013

rwaldron commented Feb 20, 2013

isaacs commented Feb 20, 2013